mcp: HTTP Header Standardization for x-mcp-header#915
mcp: HTTP Header Standardization for x-mcp-header#915guglielmo-san wants to merge 30 commits intomainfrom
Conversation
…ard request headers
…treamableClientConn
…rs for tool calls
…e comparison for parameter validation
…es and add structural validation
…ttp_standardization_2 # Conflicts: # mcp/streamable.go # mcp/streamable_client_test.go # mcp/streamable_headers.go # mcp/streamable_headers_test.go # mcp/streamable_test.go
…ernal JSON unmarshaling
…ors in ClientSession
…treamable_test.go
…_http_standardization_2' into guglielmoc/SEP-2243_http_standardization_2
guglielmo-san
changed the title
| // Avoid sending nil over the wire. | ||
| params.Arguments = map[string]any{} | ||
| } | ||
| if tool := cs.getCachedTool(params.Name); tool != nil { |
There was a problem hiding this comment.
Please also think how modelcontextprotocol/modelcontextprotocol#2549 will factor in into this solution.
|
|
||
| bodyVal := unmarshalPrimitive(argRaw) | ||
| if bodyVal == nil { | ||
| continue |
There was a problem hiding this comment.
IIUC this is a situation where the header is present, but the value in arguments is not a primitive. Please add a comment explaining why it is ok to ignore this.
There was a problem hiding this comment.
Actually it would probably be better to return an error. The SEP does not explicitly define this case but it makes more sense to me.
There was a problem hiding this comment.
Maybe we should also validate the schema on AddTool as well to make a situation where a server serves incorrect tool fail early?
There was a problem hiding this comment.
It would make sense, but it is not specified by the SEP. If the format of the Tool is wrong, then it will be filtered by the client and not returned by ListTools.
If the client still sends a non-primitive type then it will be caught here and return an error.
There was a problem hiding this comment.
I would say we have some freedom how we construct SDK APIs and their behavior. IMO that would be a valuable change and it's always easier to remove such validation than introduce it after the release.
There was a problem hiding this comment.
Ok I added the check on the server, can we consider it a non-breaking change?
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result.Tools = filterValidTools(result.Tools) |
There was a problem hiding this comment.
In general this proposal adds HTTP specific functionality, so I'm a bit surprised that the spec requires general filtering. IMO ideally this change should be contained within streamable transport as much as possible. I'll ask on the SEP for clarification.
maciej-kisiel
left a comment
maciej-kisiel
left a comment
There was a problem hiding this comment.
Also some additional comments from AI review :)
…er validation to support JSON schema annotations
| // Avoid sending nil over the wire. | ||
| params.Arguments = map[string]any{} | ||
| } | ||
| if tool := cs.getCachedTool(params.Name); tool != nil { |
There was a problem hiding this comment.
I also started wondering what should happen when the tools is not in the cache. It means that ListTools was not called before the call or the model has outdated information for some reason. Maybe we should proactively fail in that case? It likely should be consistent behavior across the SDKs, I will try to ask about it as well.
There was a problem hiding this comment.
Yes I also thought about it. I don't think we should fail on client if they do not want to call ListTools before. If the format is wrong it would fail on the server anyway, which has to be considered the ultimate source of truth
|
|
||
| bodyVal := unmarshalPrimitive(argRaw) | ||
| if bodyVal == nil { | ||
| continue |
There was a problem hiding this comment.
I would say we have some freedom how we construct SDK APIs and their behavior. IMO that would be a valuable change and it's always easier to remove such validation than introduce it after the release.
| return nil | ||
| } | ||
| result := make(map[string]string) | ||
| for propName, propSchema := range props { |
There was a problem hiding this comment.
The only comment I got on the SEP suggests that this shouldn't be limited to top level. Let's see if anyone confirms.
…nd unify primitive type string conversion
…panic on invalid header annotations
…ccess status for type validation
2 participants
Description
Implements SEP-2243 (HTTP Header Standardization) for x-mcp-param custom header.
Fixes #905